Support checkout of multiple refs for a single repository#139
Conversation
esbena
force-pushed
the
support-multiple-checkouts
branch
from
d650f33 to
56ac6df
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Thanks for doing this. It's a simple feature that was indeed missed out of the original implemtation, so it's to have it working now.
One comment/suggestion, but otherwise looks good as it is.
src/external-queries.ts
Outdated
| core.info('Checking out ' + repository); | ||
|
|
||
| const checkoutLocation = path.join(folder, repository); | ||
| const suffix = ref ? "@" + ref.replace(/\//g, '__') : ""; |
There was a problem hiding this comment.
Why have the ref as a suffix instead of having another path segment?
I had to look up to make sure @ wasn't forbidden on windows or something like that. It appears to be fine though.
Do we need to sanitise against other characters too that might be illegal in a path? I'm unsure. On the one hand it might be nice, but then the git checkout would likely fail anyway, so I don't think we actually gain much.
There was a problem hiding this comment.
Why have the ref as a suffix instead of having another path segment?
I thought we allowed the empty ref (but we don't), which would then introduce an actual problem if you mixed foo/bar and foo/bar@dev. I'll make an additional path segment instead.
Do we need to sanitise against other characters too that might be illegal in a path?
git refs are surprisingly permissive, so I think that is an uphill battle: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html. I have changed the check to avoid the classic path traversal outside of folder.
src/external-queries.test.ts
Outdated
| process.env["RUNNER_TEMP"] = tmpDir; | ||
| await externalQueries.checkoutExternalRepository("github/codeql-go", "df4c6869212341b601005567381944ed90906b6b"); | ||
| const ref = "df4c6869212341b601005567381944ed90906b6b"; | ||
| await externalQueries.checkoutExternalRepository("github/codeql-go", ref); |
There was a problem hiding this comment.
It would be nice to have this test exercise the new feature by checking out this repo at another commit as well. However I'm worried as I've seen this test sometimes time out already, so doing another checkout may be too much. I'd appreciate if you could try it out, and we'll see if it's worth it.
Ultimately it would be better if we changed this to checkout a local tiny repository instead of a medium sized repo from github.com and that would make it much faster, but that's a bigger change that I don't expect you to make here in this PR.
There was a problem hiding this comment.
The time cost is twice as high.
Besides, we now assert that a directory named df4c6869212341b601005567381944ed90906b6b exists. It would be an extraordinarily weird behaviour if we reused that for another checkout.
|
Drive-by thought: The following forbids valid paths like let tok = queryUses.split('@');
if (tok.length !== 2) {
throw new Error(getQueryUsesInvalid(configFile, queryUses));
} |
esbena
force-pushed
the
support-multiple-checkouts
branch
from
7c855c8 to
b69ef12
Compare
Good point, that should probably change to only consider the first occurrence of |
esbena
force-pushed
the
support-multiple-checkouts
branch
2 times, most recently
from
b61632c to
5874ff3
Compare
esbena
force-pushed
the
support-multiple-checkouts
branch
from
5874ff3 to
eecc25f
Compare
|
Merging on green. |
2 participants
This change ensures that two different checkouts are made of
foo/barfor the config below. The old implementation would only checkout the@devversion, and reuse that for the@legacyversion.Merge / deployment checklist